Closed
Conversation
Reviewer's GuideThis PR migrates base_geoengine to Odoo 19 by replacing the old expression API with the new Domain/Query framework and integrating geospatial operators into Field._condition_to_sql and DomainCondition, updates imports and translation calls, adds robust WKT parsing fallback, bumps module version, updates static assets and tests. Class diagram for geospatial operator integration in DomainCondition and FieldclassDiagram
class DomainCondition {
+field_expr: str
+operator: str
+value
+_opt_level: OptimizationLevel
+checked()
+_to_sql(model, alias, query)
+_optimize_step(model, level)
}
class Field {
+_condition_to_sql(field_expr, operator, value, model, alias, query)
}
class GeoOperator {
+get_geo_greater_sql(...)
+get_geo_lesser_sql(...)
+get_geo_equal_sql(...)
+get_geo_touch_sql(...)
+get_geo_within_sql(...)
+get_geo_contains_sql(...)
+get_geo_intersect_sql(...)
}
class GeoField {
+geo_type
+srid
+dim
+update_geo_db_column(model)
+update_db_column(model, column)
+entry_to_shape(value, same_type)
}
Field <|-- GeoField
DomainCondition --> Field : uses
Field --> GeoOperator : instantiates
GeoField --> GeoOperator : instantiates
Class diagram for new domains.py monkey patching and operator extensionclassDiagram
class Domain {
+AND(domains)
}
class DomainCondition {
+checked()
+_to_sql(model, alias, query)
+_optimize_step(model, level)
}
class domains {
+CONDITION_OPERATORS
}
DomainCondition --> domains : extends CONDITION_OPERATORS
Domain --> DomainCondition : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The direct monkey-patching of core methods like Field._condition_to_sql and DomainCondition._to_sql/_optimize_step is quite complex and brittle—consider refactoring into a single, centralized extension or using official hooks to reduce risk.
- You have duplicated definitions of GEO_OPERATORS and GEO_SQL_OPERATORS in multiple files—extract them into one shared module to avoid drift and simplify maintenance.
- The value_to_shape function silently falls back to an empty POINT(0.0 0.0) on parse errors, which can mask real data issues—consider surfacing errors or handling invalid WKT more explicitly instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The direct monkey-patching of core methods like Field._condition_to_sql and DomainCondition._to_sql/_optimize_step is quite complex and brittle—consider refactoring into a single, centralized extension or using official hooks to reduce risk.
- You have duplicated definitions of GEO_OPERATORS and GEO_SQL_OPERATORS in multiple files—extract them into one shared module to avoid drift and simplify maintenance.
- The value_to_shape function silently falls back to an empty POINT(0.0 0.0) on parse errors, which can mask real data issues—consider surfacing errors or handling invalid WKT more explicitly instead.
## Individual Comments
### Comment 1
<location> `base_geoengine/fields.py:312` </location>
<code_context>
def update_geo_db_column(self, model):
"""Update the column type in the database."""
- cr = model._cr
+ cr = model.env.cr
query = """SELECT srid, type, coord_dimension
FROM geometry_columns
</code_context>
<issue_to_address>
**question (bug_risk):** Switching from model._cr to model.env.cr may have side effects.
Please verify that all usages of this method are compatible with model.env.cr and do not rely on a different cursor.
</issue_to_address>
### Comment 2
<location> `base_geoengine/geo_convertion_helper.py:29-38` </location>
<code_context>
+from .geo_operators import GeoOperator
logger = logging.getLogger(__name__)
try:
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning a default geometry on WKT parse failure may mask data issues.
Instead of returning a default geometry, raise an exception or log the error to make data issues visible and easier to debug.
</issue_to_address>
### Comment 3
<location> `base_geoengine/models/geo_vector_layer.py:6` </location>
<code_context>
# Copyright 2023 ACSONE SA/NV
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
-from odoo import _, api, fields, models
+from odoo import api, fields, models
from odoo.exceptions import ValidationError
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Removing '_' import requires all translations to use self.env._.
Check for any remaining uses of '_' for translations and update them to 'self.env._' to prevent runtime errors.
Suggested implementation:
```python
raise ValidationError(
self.env._(
```
If there are other uses of `_(` for translations elsewhere in this file, they should also be replaced with `self.env._(`. Please review the entire file to ensure all translation calls use `self.env._`.
</issue_to_address>
### Comment 4
<location> `base_geoengine/fields.py:36` </location>
<code_context>
)
-
-
-def get_geo_func(current_operator, operator, left, value, params, table):
- """
- This method will call the SQL query corresponding to the requested geo operator
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the geo operator logic by merging mappings, using dynamic dispatch, and extracting repeated code into helpers.
```suggestion
Most of this boilerplate can be collapsed into a few small helpers. In particular:
1. Remove the huge `match` in `get_geo_func` by dynamic dispatch.
2. Merge `GEO_OPERATORS` and `GEO_SQL_OPERATORS` into one mapping.
3. Replace your custom `where_calc` with directly using `Domain._to_sql`.
4. Factor out the repeated random‐alias/subquery logic into a helper.
For example:
```python
# 1. Single OPERATOR map for both the SQL symbol and the GeoOperator method suffix
GEO_OPERATORS = {
"geo_greater": { "sql": SQL(">"), "method": "get_geo_greater_sql" },
"geo_lesser": { "sql": SQL("<"), "method": "get_geo_lesser_sql" },
"geo_equal": { "sql": SQL("="), "method": "get_geo_equal_sql" },
# ... etc ...
}
# 2. Short dynamic get_geo_func:
def get_geo_sql(operator_obj, operator, field, value, params, table):
cfg = GEO_OPERATORS.get(operator)
if not cfg:
raise NotImplementedError(f"Unsupported geo operator {operator}")
return getattr(operator_obj, cfg["method"])(table, field, value, params)
# 3. Replace custom where_calc with Domain._to_sql
def where_query(model, domain, alias=None):
q = Query(model.env, alias, model._table)
if not domain:
return q
dom = Domain(domain).optimize_full(model)
cond = dom._to_sql(model, alias, q)
q.add_where(cond)
return q
# 4. Extract the subquery builder
def build_geo_subquery(env, rel_model_name, ref_domain, base_alias, field, operator):
rel_model = env[rel_model_name]
rel_alias = f"{rel_model._table}_{env.cr.dbname[:3]}" # or your own alias logic
rel_q = where_query(rel_model, ref_domain, alias=rel_alias)
op = GEO_OPERATORS[operator]["sql"]
rel_q.add_where(f'{op}("{base_alias}"."{field}", {rel_alias}.{field})')
sql_code = rel_q.subselect("1")
mog = env.cr.mogrify(sql_code.code, sql_code.params).decode()
return f"EXISTS({mog.replace('%', '%%')})"
# 5. Simplify the patched _condition_to_sql
original = Field._condition_to_sql
def _condition_to_sql(self, field_expr, operator, value, model, alias, query):
if operator in GEO_OPERATORS:
current = GeoOperator(model._fields[field_expr])
params = []
if isinstance(value, dict):
clauses = []
for rel, dom in value.items():
m, col = rel.rsplit(".", 1)
clauses.append(build_geo_subquery(
model.env, m, dom, alias, field_expr, operator
))
return SQL(" AND ".join(clauses))
else:
sql_str = get_geo_sql(current, operator, field_expr, value, params, model._table)
return SQL(sql_str, *params)
return original(self, field_expr, operator, value, model, alias, query)
Field._condition_to_sql = _condition_to_sql
```
These changes:
- Collapse the two dicts into one.
- Remove the `match`/`case`.
- Use `Domain._to_sql` directly instead of a full copy of `where_calc`.
- Extract subquery/alias logic into `build_geo_subquery`.
</issue_to_address>
### Comment 5
<location> `base_geoengine/domains.py:39` </location>
<code_context>
+)
+
+
+def checked(self) -> DomainCondition:
+ """Validate `self` and return it if correct, otherwise raise an exception."""
+ if not isinstance(self.field_expr, str) or not self.field_expr:
</code_context>
<issue_to_address>
**issue (complexity):** Consider wrapping core DomainCondition methods to intercept only geo operators and delegate all other logic to the original implementations.
```markdown
You can drastically reduce duplication and future‐proof against Odoo core changes by wrapping (not re-implementing) the core `DomainCondition` methods. Only intercept your geo operators and defer everything else to the original implementations:
```python
from functools import wraps
from odoo.orm.domains import DomainCondition, OptimizationLevel
GEO_OPERATORS = {
"geo_greater", "geo_lesser", "geo_equal", "geo_touch",
"geo_within", "geo_contains", "geo_intersect",
}
# 1) Wrap checked()
_orig_checked = DomainCondition.checked
@wraps(_orig_checked)
def _checked(self):
dc = _orig_checked(self) # run all built‐in checks first
if dc.operator in GEO_OPERATORS:
# here you only do the extra geo validation/normalization
with contextlib.suppress(Exception):
field = dc._field(dc.model)
if not hasattr(field, "geo_type"):
dc._raise("Invalid geo operator on non‐geo field")
return dc
# 2) Wrap _to_sql()
_orig_to_sql = DomainCondition._to_sql
@wraps(_orig_to_sql)
def _to_sql(self, model, alias, query):
if self.operator in GEO_OPERATORS:
field = self._field(model)
model._check_field_access(field, "read")
return field.condition_to_sql(
self.field_expr, self.operator, self.value, model, alias, query
)
return _orig_to_sql(self, model, alias, query)
# 3) Wrap _optimize_step()
_orig_optimize = DomainCondition._optimize_step
@wraps(_orig_optimize)
def _optimize_step(self, model, level):
if self.operator in GEO_OPERATORS and level < OptimizationLevel.FULL:
# mark fully optimized & keep as-is
optimized = DomainCondition(self.field_expr, self.operator, self.value)
object.__setattr__(optimized, "_opt_level", OptimizationLevel.FULL)
return optimized
return _orig_optimize(self, model, level)
# Apply patches
DomainCondition.checked = _checked
DomainCondition._to_sql = _to_sql
DomainCondition._optimize_step = _optimize_step
# Only extend the operator set, no wholesale rewrite
from odoo.orm import domains
domains.CONDITION_OPERATORS |= GEO_OPERATORS
```
This approach
- Preserves all upstream logic (avoids bit-rot when Odoo changes core methods)
- Only adds your geo branches
- Keeps each wrapper tiny and focused
- Maintains full functionality without re-implementing existing checks.
</issue_to_address>
### Comment 6
<location> `base_geoengine/domains.py:55-59` </location>
<code_context>
if operator not in CONDITION_OPERATORS:
# MOD
if operator not in GEO_OPERATORS:
self._raise("Invalid operator")
# /MOD
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if operator not in CONDITION_OPERATORS and operator not in GEO_OPERATORS:
self._raise("Invalid operator")
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 7
<location> `base_geoengine/fields.py:66-70` </location>
<code_context>
if model._active_name and active_test and model._context.get("active_test", True):
# the item[0] trick below works for domain items and '&'/'|'/'!'
# operators too
if not any(item[0] == model._active_name for item in domain):
domain = [(model._active_name, "=", 1)] + domain
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if model._active_name and active_test and model._context.get("active_test", True) and not any(item[0] == model._active_name for item in domain):
domain = [(model._active_name, "=", 1)] + domain
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 8
<location> `base_geoengine/fields.py:69` </location>
<code_context>
def where_calc(self, model, domain, active_test=True, alias=None):
"""
This method is copied from base, we need to create our own query.
"""
# if the object has an active field ('active', 'x_active'), filter out all
# inactive records unless they were explicitly asked for
if model._active_name and active_test and model._context.get("active_test", True):
# the item[0] trick below works for domain items and '&'/'|'/'!'
# operators too
if not any(item[0] == model._active_name for item in domain):
domain = [(model._active_name, "=", 1)] + domain
query = Query(model.env, alias, model._table)
if domain:
# MOD # old expression.expression
# _to_sql(self, model: BaseModel, alias: str, query: Query)
# DomainCondition(self.field_expr, operator, value)
# domain_condition = DomainCondition(domain[0][0], domain[0][1], domain[0][2])
# optimize the domain condition
# domain_condition = domain_condition.optimize_full(model)
# res = domain_condition._to_sql(model=model, alias=alias, query=query)
# return query
# return expression.expression(domain, model, alias=alias, query=query).query
# In Odoo 19, create Domain object and use its _to_sql method
domain_obj = Domain(domain)
optimized_domain = domain_obj.optimize_full(model)
# Apply domain to query using _to_sql
sql_condition = optimized_domain._to_sql(model, alias, query)
query.add_where(sql_condition)
return query
return query
</code_context>
<issue_to_address>
**suggestion (code-quality):** Invert any/all to simplify comparisons ([`invert-any-all`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/invert-any-all/))
```suggestion
if all(item[0] != model._active_name for item in domain):
```
</issue_to_address>
### Comment 9
<location> `base_geoengine/fields.py:121` </location>
<code_context>
def _condition_to_sql(
self,
field_expr: str,
operator: str,
value,
model: BaseModel,
alias: str,
query: Query,
) -> SQL:
"""
This method has been monkey patched in order to be able to include
geo_operators into the Odoo search method.
In Odoo 19, _condition_to_sql moved from BaseModel to Field.
"""
# print(field_expr)
# field_expr = self.name # In Field context, self.name is the field name
if operator in GEO_OPERATORS.keys():
current_field = model._fields.get(field_expr)
current_operator = GeoOperator(current_field)
if current_field and isinstance(current_field, GeoField):
params = []
if isinstance(value, dict):
# We are having indirect geo_operator like (‘geom’, ‘geo_...’,
# {‘res.zip.poly’: [‘id’, ‘in’, [1,2,3]] })
ref_search = value
sub_queries = []
for key in ref_search:
i = key.rfind(".")
rel_model_name = key[0:i]
rel_col = key[i + 1 :]
rel_model = model.env[rel_model_name]
# we compute the attributes search on spatial rel
if ref_search[key]:
rel_alias = (
rel_model._table
+ "_"
+ "".join(random.choices(string.ascii_lowercase, k=5))
)
rel_query = where_calc(
self,
rel_model,
ref_search[key],
active_test=True,
alias=rel_alias,
)
# rel_model._apply_ir_rules(rel_query, "read")
if operator == "geo_equal":
rel_query.add_where(
f'"{alias}"."{field_expr}" {GEO_OPERATORS[operator]} '
f"{rel_alias}.{rel_col}"
)
elif operator in ("geo_greater", "geo_lesser"):
rel_query.add_where(
f"ST_Area({alias}.{field_expr}) "
f"{GEO_OPERATORS[operator]} "
f"ST_Area({rel_alias}.{rel_col})"
)
else:
rel_query.add_where(
f'{GEO_OPERATORS[operator]}("{alias}"."{field_expr}", '
f"{rel_alias}.{rel_col})"
)
subquery_sql = rel_query.subselect("1")
sub_query_mogrified = (
model.env.cr.mogrify(subquery_sql.code, subquery_sql.params)
.decode("utf-8")
.replace(f"'{rel_model._table}'", f'"{rel_model._table}"')
.replace("%", "%%")
)
sub_queries.append(f"EXISTS({sub_query_mogrified})")
query_str = " AND ".join(sub_queries)
else:
query_str = get_geo_func(
current_operator, operator, field_expr, value, params, model._table
)
return SQL(query_str, *params)
return original___condition_to_sql(
self,
field_expr=field_expr,
operator=operator,
value=value,
model=model,
alias=alias,
query=query,
)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] ([`remove-redundant-slice-index`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-slice-index/))
- Use set when checking membership of a collection of literals ([`collection-into-set`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/collection-into-set/))
- Low code quality found in \_condition\_to\_sql - 16% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 10
<location> `base_geoengine/fields.py:394-403` </location>
<code_context>
def update_db_column(self, model, column):
"""Create/update the column corresponding to ``self``.
For creation of geo column
:param model: an instance of the field's model
:param column: the column's configuration (dict)
if it exists, or ``None``
"""
# the column does not exist, create it
if not column:
create_geo_column(
model.env.cr,
model._table,
self.name,
self.geo_type.upper(),
self.srid,
self.dim,
self.string,
)
if self.gist_index:
create_geo_index(model.env.cr, self.name, model._table)
return
if column["udt_name"] == self.column_type[0]:
if self.gist_index:
create_geo_index(model.env.cr, self.name, model._table)
return
self.update_geo_db_column(model)
if column["udt_name"] in self.column_cast_from:
sql.convert_column(
model.env.cr, model._table, self.name, self.column_type[1]
)
else:
newname = (self.name + "_moved{}").format
i = 0
while sql.column_exists(model.env.cr, model._table, newname(i)):
i += 1
if column["is_nullable"] == "NO":
sql.drop_not_null(model.env.cr, model._table, self.name)
sql.rename_column(model.env.cr, model._table, self.name, newname(i))
sql.create_column(
model.env.cr, model._table, self.name, self.column_type[1], self.string
)
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 11
<location> `base_geoengine/models/geo_vector_layer.py:100` </location>
<code_context>
@api.constrains("geo_field_id", "model_id")
def _check_geo_field_id(self):
for rec in self:
if rec.model_id:
if not rec.geo_field_id.model_id == rec.model_id:
raise ValidationError(
self.env._(
"The geo_field_id must be a field in %s model",
rec.model_id.display_name,
)
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Simplify logical expression using De Morgan identities ([`de-morgan`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/de-morgan/))
```suggestion
if rec.geo_field_id.model_id != rec.model_id:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
27
to
38
| elif use_wkb: | ||
| return wkb.loads(value, hex=True) | ||
| else: | ||
| return wkt.loads(value) | ||
| # <NIKMOD> | ||
| # 'POINT(0.0 0.0)' | ||
|
|
||
| try: | ||
| return wkt.loads(value) | ||
| except Exception as e: | ||
| logger.warning(_("Failed to parse WKT: %s", e)) # pylint: disable=prefer-env-translation |
There was a problem hiding this comment.
issue (bug_risk): Returning a default geometry on WKT parse failure may mask data issues.
Instead of returning a default geometry, raise an exception or log the error to make data issues visible and easier to debug.
base_geoengine/fields.py
Outdated
| logger.warning("Shapely or geojson are not available in the sys path") | ||
|
|
||
|
|
||
| def get_geo_func(current_operator, operator, left, value, params, table): |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the geo operator logic by merging mappings, using dynamic dispatch, and extracting repeated code into helpers.
Suggested change
| def get_geo_func(current_operator, operator, left, value, params, table): | |
| Most of this boilerplate can be collapsed into a few small helpers. In particular: | |
| 1. Remove the huge `match` in `get_geo_func` by dynamic dispatch. | |
| 2. Merge `GEO_OPERATORS` and `GEO_SQL_OPERATORS` into one mapping. | |
| 3. Replace your custom `where_calc` with directly using `Domain._to_sql`. | |
| 4. Factor out the repeated random‐alias/subquery logic into a helper. | |
| For example: | |
| ```python | |
| # 1. Single OPERATOR map for both the SQL symbol and the GeoOperator method suffix | |
| GEO_OPERATORS = { | |
| "geo_greater": { "sql": SQL(">"), "method": "get_geo_greater_sql" }, | |
| "geo_lesser": { "sql": SQL("<"), "method": "get_geo_lesser_sql" }, | |
| "geo_equal": { "sql": SQL("="), "method": "get_geo_equal_sql" }, | |
| # ... etc ... | |
| } | |
| # 2. Short dynamic get_geo_func: | |
| def get_geo_sql(operator_obj, operator, field, value, params, table): | |
| cfg = GEO_OPERATORS.get(operator) | |
| if not cfg: | |
| raise NotImplementedError(f"Unsupported geo operator {operator}") | |
| return getattr(operator_obj, cfg["method"])(table, field, value, params) | |
| # 3. Replace custom where_calc with Domain._to_sql | |
| def where_query(model, domain, alias=None): | |
| q = Query(model.env, alias, model._table) | |
| if not domain: | |
| return q | |
| dom = Domain(domain).optimize_full(model) | |
| cond = dom._to_sql(model, alias, q) | |
| q.add_where(cond) | |
| return q | |
| # 4. Extract the subquery builder | |
| def build_geo_subquery(env, rel_model_name, ref_domain, base_alias, field, operator): | |
| rel_model = env[rel_model_name] | |
| rel_alias = f"{rel_model._table}_{env.cr.dbname[:3]}" # or your own alias logic | |
| rel_q = where_query(rel_model, ref_domain, alias=rel_alias) | |
| op = GEO_OPERATORS[operator]["sql"] | |
| rel_q.add_where(f'{op}("{base_alias}"."{field}", {rel_alias}.{field})') | |
| sql_code = rel_q.subselect("1") | |
| mog = env.cr.mogrify(sql_code.code, sql_code.params).decode() | |
| return f"EXISTS({mog.replace('%', '%%')})" | |
| # 5. Simplify the patched _condition_to_sql | |
| original = Field._condition_to_sql | |
| def _condition_to_sql(self, field_expr, operator, value, model, alias, query): | |
| if operator in GEO_OPERATORS: | |
| current = GeoOperator(model._fields[field_expr]) | |
| params = [] | |
| if isinstance(value, dict): | |
| clauses = [] | |
| for rel, dom in value.items(): | |
| m, col = rel.rsplit(".", 1) | |
| clauses.append(build_geo_subquery( | |
| model.env, m, dom, alias, field_expr, operator | |
| )) | |
| return SQL(" AND ".join(clauses)) | |
| else: | |
| sql_str = get_geo_sql(current, operator, field_expr, value, params, model._table) | |
| return SQL(sql_str, *params) | |
| return original(self, field_expr, operator, value, model, alias, query) | |
| Field._condition_to_sql = _condition_to_sql |
These changes:
- Collapse the two dicts into one.
- Remove the
match/case. - Use
Domain._to_sqldirectly instead of a full copy ofwhere_calc. - Extract subquery/alias logic into
build_geo_subquery.
base_geoengine/fields.py
Outdated
| original___condition_to_sql = Field._condition_to_sql | ||
|
|
||
|
|
||
| def _condition_to_sql( |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] (
remove-redundant-slice-index) - Use set when checking membership of a collection of literals (
collection-into-set) - Low code quality found in _condition_to_sql - 16% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
Comment on lines
394
to
403
| newname = (self.name + "_moved{}").format | ||
| i = 0 | ||
| while sql.column_exists(model._cr, model._table, newname(i)): | ||
| while sql.column_exists(model.env.cr, model._table, newname(i)): | ||
| i += 1 | ||
| if column["is_nullable"] == "NO": | ||
| sql.drop_not_null(model._cr, model._table, self.name) | ||
| sql.rename_column(model._cr, model._table, self.name, newname(i)) | ||
| sql.drop_not_null(model.env.cr, model._table, self.name) | ||
| sql.rename_column(model.env.cr, model._table, self.name, newname(i)) | ||
| sql.create_column( | ||
| model._cr, model._table, self.name, self.column_type[1], self.string | ||
| model.env.cr, model._table, self.name, self.column_type[1], self.string | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
| @@ -99,7 +99,7 @@ def _check_geo_field_id(self): | |||
| if rec.model_id: | |||
| if not rec.geo_field_id.model_id == rec.model_id: | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
weinni2000
force-pushed
the
19.0.preparation
branch
4 times, most recently
from
9f4e987 to
f7a3db9
Compare
weinni2000
force-pushed
the
19.0.preparation
branch
from
f7a3db9 to
0811a2a
Compare
Owner
Author
|
not current |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Upgrade base_geoengine for Odoo 19 by refactoring search condition generation to support geospatial operators in domains, updating imports and APIs for compatibility, improving WKT parsing resilience, aligning tests with the new search API, and refreshing documentation and manifest metadata for version 19.0
New Features:
geo_greater,geo_lesser,geo_equal,geo_touch,geo_within,geo_contains, andgeo_intersectoperatorsEnhancements:
Documentation:
Tests:
limitparameter in search calls and rename test methods to align with Odoo 19 search APIChores: